Skip to content

HTTPCLIENT-2358 Implement a mutual authentication capable SPNEGO scheme#615

Open
stoty wants to merge 15 commits into
apache:masterfrom
stoty:HTTPCLIENT-2358
Open

HTTPCLIENT-2358 Implement a mutual authentication capable SPNEGO scheme#615
stoty wants to merge 15 commits into
apache:masterfrom
stoty:HTTPCLIENT-2358

Conversation

@stoty

@stoty stoty commented Jan 23, 2025

Copy link
Copy Markdown
Contributor

No description provided.

@stoty stoty marked this pull request as draft January 23, 2025 09:58
@ok2c

ok2c commented Jan 23, 2025

Copy link
Copy Markdown
Member

@stoty I cannot contribute here much. My only wish, and I understand it is a big ask, to also create a compatibility test similar to those we have for BASIC / DIGEST with Squid and Apache HTTPD

https://github.com/apache/httpcomponents-client/tree/master/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/compatibility

@stoty

stoty commented Jan 23, 2025

Copy link
Copy Markdown
Contributor Author

I was afraid you're going to say that...

There are two ways to go about that:

  • Try to find an docker image with a KDC and web server, and model the test on the Squid test
  • Initialize a local (non-dockerized) KDC with Kerby, start Jetty, and use that.

I don't know which of the two is more work, but I know where to copy the test setup from for the second one.

Do you have a preference ?

@ok2c

ok2c commented Jan 23, 2025

Copy link
Copy Markdown
Member

I was afraid you're going to say that...

@stoty Kek.

Anyways, We already test for compatibility with Jetty in core by running it in a Docker container

https://github.com/apache/httpcomponents-core/blob/master/httpcore5-testing/src/test/java/org/apache/hc/core5/testing/compatibility/JettyCompatIT.java

I t would be my preferred option but I do not know how difficult it is to pack extra Jetty dependencies into a Docker container. I was also hoping Apache HTTPD or Ngnix might have Kerberos support.

If it is too much effort, disregard my request.

@stoty

stoty commented Jan 23, 2025

Copy link
Copy Markdown
Contributor Author

It is a reasonable request, and it IS helpful to catch any regressions, etc.
I will try to find and existing docker image that is close enough.

@garydgregory

Copy link
Copy Markdown
Member

Apache Kerby implements Kerberos, you can get inspiration (copy) how they do it: https://directory.apache.org/kerby/

@stoty

stoty commented Jan 24, 2025

Copy link
Copy Markdown
Contributor Author

Kerby is great for in-JVM tests, but for dockerized tests it's probably easier to use MIT kerberos.

@stoty

stoty commented Jan 24, 2025

Copy link
Copy Markdown
Contributor Author

After poking around a bit, it seems that neither Apache Httpd, not Nginx supports SPNEGO out of the box.
While both have third party SPNEGO modules, creating a test image with a full working setup looks like a lot of work.

For now, I plan to make a local test with Kerby + Jetty, without Docker.

@michael-o

michael-o commented Jan 28, 2025

Copy link
Copy Markdown
Member

A full coverage test contains JGSS (via Tomcat my SpnegoAuthenticator), MIT Kerberos (via mod_auth_gssapi), Microsoft Kerberos (via IIS (SSPI)). Virtually impossible to automate. I will do manual testing anyway.

I have everything in place at work to test.

@michael-o michael-o left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much less code review. Thanks. I already see some points.

@stoty

stoty commented Jan 28, 2025

Copy link
Copy Markdown
Contributor Author

Thanks.
I've made the changes to MutualKerberosConfig.

While I don't use Tomcat, testing Jgss with Jetty is doable, in fact one of the ways I tested this was via the SPNEGO test in HttpCLient->Calcite Avatica->Phoenix PQS.
It does bring in a ton of extra test dependencies, and I couldn't really find where to fit the that integration test in the project.

@stoty stoty force-pushed the HTTPCLIENT-2358 branch 6 times, most recently from 2d7dc5e to 5a11bce Compare February 20, 2025 23:00
@stoty

stoty commented Feb 20, 2025

Copy link
Copy Markdown
Contributor Author

I have added tests for direct connection to httpd + mod_auth_spnego, @michael-o .
The tests have immediately found a problem, which I also fixed.

The async client cannot use the Subject set with Subject.callAs to figure out the Kerberos credentials (probably due to the auth being run from a different thread)
Is that acceptable, or should we extract and store the Subject ?

Async still works if we create and set KerbersCredentials. (as in the new tests I added)

@stoty stoty marked this pull request as ready for review February 22, 2025 19:39
@stoty

stoty commented Feb 22, 2025

Copy link
Copy Markdown
Contributor Author

I have added tests for SPNEGO authentication for Squid.

I feel that this is ready now, @michael-o .

@stoty stoty requested a review from michael-o February 22, 2025 19:42
@michael-o

Copy link
Copy Markdown
Member

I have added tests for direct connection to httpd + mod_auth_spnego, @michael-o . The tests have immediately found a problem, which I also fixed.

The async client cannot use the Subject set with Subject.callAs to figure out the Kerberos credentials (probably due to the auth being run from a different thread) Is that acceptable, or should we extract and store the Subject ?

Async still works if we create and set KerbersCredentials. (as in the new tests I added)

Which bug exactly?

@michael-o michael-o left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am picking this up again and want to complete next month.
Structural issues:

  • Since in both auth and impl.path packages contains multiple new classes there should be in respective new subpackages gss for tidiness.
  • Don't use the term "Kerberos" in classnames becausee we perform SPNEGO with GSS-API, correctly it should be called "Gss"
  • SPNEGO in class names should be written consistently, In PascalCase and not mixed case.
  • Drop the prefix "Mutual" in class names altogether because one can disable on request making it a contradiction and the other party could pontentially deny it.

@stoty

stoty commented Feb 27, 2025

Copy link
Copy Markdown
Contributor Author

I can do that, but that would mean the the three formerly mutual classes only differ in case and package from the old deprecated classes. I can see that causing a lot of gried for users down the line.

  • Since in both auth and impl.path packages contains multiple new classes there should be in respective new subpackages gss for tidiness.

Sure.

  • Don't use the term "Kerberos" in classnames becausee we perform SPNEGO with GSS-API, correctly it should be called "Gss"

Sure.

  • SPNEGO in class names should be written consistently, In PascalCase and not mixed case.

Only the old deprecated classes use SPNego, and fixing those would make it even easier to mix them up with the new ones. (apart from breaking backwards compatibility, if we care about that)

  • Drop the prefix "Mutual" in class names altogether because one can disable on request making it a contradiction and the other party could pontentially deny it.

I can do that, but then then the only difference between the old and new classes will be the packages and the case.
Machines don't care, but I see that causing grief for users.

@stoty

stoty commented Feb 27, 2025

Copy link
Copy Markdown
Contributor Author

Thanks for the review.
I have made the suggested changes, but I am not happy with how close the old new class names are.

I have also left KerberosCredentials alone, because it is used by both the new and old code.
Should I make an identical child class with GssCredentials name ?

@michael-o

Copy link
Copy Markdown
Member

I can do that, but that would mean the the three formerly mutual classes only differ in case and package from the old deprecated classes. I can see that causing a lot of gried for users down the line.

  • Since in both auth and impl.path packages contains multiple new classes there should be in respective new subpackages gss for tidiness.

Sure.

  • Don't use the term "Kerberos" in classnames becausee we perform SPNEGO with GSS-API, correctly it should be called "Gss"

Sure.

  • SPNEGO in class names should be written consistently, In PascalCase and not mixed case.

Only the old deprecated classes use SPNego, and fixing those would make it even easier to mix them up with the new ones. (apart from breaking backwards compatibility, if we care about that)

  • Drop the prefix "Mutual" in class names altogether because one can disable on request making it a contradiction and the other party could pontentially deny it.

I can do that, but then then the only difference between the old and new classes will be the packages and the case. Machines don't care, but I see that causing grief for users.

Don't rename old ones, only new ones. If old ones are reused maye it would be better to copy them?

@michael-o

Copy link
Copy Markdown
Member

Thanks for the review. I have made the suggested changes, but I am not happy with how close the old new class names are.

I have also left KerberosCredentials alone, because it is used by both the new and old code. Should I make an identical child class with GssCredentials name ?

Yes, please. Don't break old deprecated, but add new classes for the new impl.

@ok2c

ok2c commented Oct 17, 2025

Copy link
Copy Markdown
Member

@michael-o Any chance this feature could make it into 5.6?

@michael-o

Copy link
Copy Markdown
Member

@michael-o Any chance this feature could make it into 5.6?

Do you have a timeframe for 5.6?

@ok2c

ok2c commented Oct 18, 2025

Copy link
Copy Markdown
Member

@michael-o Any chance this feature could make it into 5.6?

Do you have a timeframe for 5.6?

@michael-o I would like to release 5.6-alpha1 next week. However, a GA is unlikely sooner than Q2, so there is still some time, given this feature has no major API changes and can be added quite late in the release cycle.

@michael-o

Copy link
Copy Markdown
Member

@michael-o Any chance this feature could make it into 5.6?

Do you have a timeframe for 5.6?

@michael-o I would like to release 5.6-alpha1 next week. However, a GA is unlikely sooner than Q2, so there is still some time, given this feature has no major API changes and can be added quite late in the release cycle.

You mean 2026Q2?

@ok2c

ok2c commented Oct 18, 2025

Copy link
Copy Markdown
Member

You mean 2026Q2?

@michael-o Yes.

@stoty

stoty commented Oct 20, 2025

Copy link
Copy Markdown
Contributor Author

I realized that the SpnegoUtil stuff is not really related to the new SPNEGO implementation, it is needed for JDK18+.

Should I split that to a new ticket/PR ?

@ok2c

ok2c commented Oct 20, 2025

Copy link
Copy Markdown
Member

@stoty You may still be stuck with @michael-o as a reviewer, but generally smaller change-sets are always easier to review and digest.

@stoty

stoty commented Oct 20, 2025

Copy link
Copy Markdown
Contributor Author

My bad, SubjectUtil is only used in the test code I added, so it doesn't make a lot of sense to add it in a separate ticket.

@stoty

stoty commented Oct 20, 2025

Copy link
Copy Markdown
Contributor Author

There is a separate (pre-existing) issue with JDK22+ and Async SPNEGO.

I have opened https://issues.apache.org/jira/browse/HTTPCLIENT-2402 to track it,
and will open a follow-up PR once this one has been merged.
(As testing for the problem depends on the test infra added in this patch)

@stoty

stoty commented Oct 21, 2025

Copy link
Copy Markdown
Contributor Author

I could factor out the ProtocolExec/AsyncProtocolExec and AuthenticationHandler changes into a separate JIRA, as those fix a bug in HTTPCLIENT-2356, and are not directly related to the new Scheme. Should I do that ?

@stoty

stoty commented Oct 21, 2025

Copy link
Copy Markdown
Contributor Author

I have decided to create a separate JIRA for the proxy mutual auth issue mentioned above.
#745
HTTPCLIENT-2403

Please take a look.
If that one is merged, I will rebase this PR.

@michael-o

Copy link
Copy Markdown
Member

Folks, I have stalled this for now because of time and personal issues. I hope I can pick this up at a later point in time. Not forgotten, but not a priority for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants